Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add more config parameters to HTTPClientSettings to be used by multiple http-based exporters #4188

Merged
merged 9 commits into from
Dec 6, 2021
Merged

Add more config parameters to HTTPClientSettings to be used by multiple http-based exporters #4188

merged 9 commits into from
Dec 6, 2021

Conversation

rrupapara
Copy link
Contributor

Add more config parameters to HTTPClientSettings to be used by multiple http-based exporters

Description: Added 3 more config parameters MaxIdleConns, MaxIdleConnsPerHost and IdleConnTimeout in HTTPClientSettings

Link to tracking Issue: open-telemetry/opentelemetry-collector-contrib#5432 (comment)

Testing: All test cases are passing

@rrupapara rrupapara marked this pull request as ready for review October 13, 2021 12:41
@rrupapara rrupapara requested review from a team and Aneurysm9 October 13, 2021 12:41
jcchavezs
jcchavezs suggested changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@jcchavezs jcchavezs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoid doing this logic in tests (same as it would be avoided in non test code) https://github.com/open-telemetry/opentelemetry-collector/pull/4188/files#diff-ae54372cbf36c6ca3ce262dc39d7db67e1d71bba20c8838a9906bc123133c8b6R114

@rrupapara
Copy link
Contributor Author

@Aneurysm9 Can you please help with the approval of this PR?

@dmitryax
Copy link
Member

please fix linter issues

@rrupapara
Copy link
Contributor Author

please fix linter issues

@dmitryax fixed the linting issue, please review the same.

@codecov
Copy link

codecov bot commented Oct 30, 2021 •
edited
Loading

Codecov Report

Merging #4188 (26ba6f0) into main (acf6556) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head 26ba6f0 differs from pull request most recent head c27c673. Consider uploading reports for the commit c27c673 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4188      +/-   ##
==========================================
+ Coverage   90.68%   90.79%   +0.10%     
==========================================
  Files         179      179              
  Lines       10425    10424       -1     
==========================================
+ Hits         9454     9464      +10     
+ Misses        755      743      -12     
- Partials      216      217       +1
Impacted Files Coverage Δ
config/confighttp/confighttp.go 100.00% <100.00%> (ø)
config/configmap.go 82.14% <0.00%> (-3.13%) ⬇️
exporter/otlpexporter/otlp.go 72.83% <0.00%> (-1.58%) ⬇️
exporter/otlphttpexporter/otlp.go 82.05% <0.00%> (-0.88%) ⬇️
internal/otlptext/logs.go 100.00% <0.00%> (ø)
internal/otlptext/traces.go 100.00% <0.00%> (ø)
internal/otlptext/metrics.go 100.00% <0.00%> (ø)
config/internal/configsource/manager.go 87.42% <0.00%> (+0.03%) ⬆️
config/configtest/configtest.go 93.61% <0.00%> (+2.61%) ⬆️
config/configmapprovider/file.go 100.00% <0.00%> (+15.78%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acf6556...c27c673. Read the comment docs.

@carlosalberto
Copy link
Contributor

Probably an overkill, but all the values could be pointers, in order to keep things uniform. Likewise, only set the values IF they are explicitly set, else ignore.

@rrupapara
Copy link
Contributor Author

Probably an overkill, but all the values could be pointers, in order to keep things uniform. Likewise, only set the values IF they are explicitly set, else ignore.

Do you mean to also have these two parameters(MaxIdleConnsPerHost, MaxConnsPerHost) as pointers variables?

@rrupapara
Copy link
Contributor Author

@carlosalberto Could you plz confirm the above?

@carlosalberto
Copy link
Contributor

Do you mean to also have these two parameters(MaxIdleConnsPerHost, MaxConnsPerHost) as pointers variables?

That's correct (and sorry for the delay!) @bogdandrutu what do you think?

@bogdandrutu
Copy link
Member

/easycla

@bogdandrutu bogdandrutu merged commit a06ca26 into open-telemetry:main Dec 6, 2021
@hprajapati-splunk hprajapati-splunk mentioned this pull request Dec 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hprajapati-splunk hprajapati-splunk hprajapati-splunk left review comments

@bogdandrutu bogdandrutu bogdandrutu left review comments

@carlosalberto carlosalberto carlosalberto left review comments

@rmfitzpatrick rmfitzpatrick rmfitzpatrick approved these changes

@dmitryax dmitryax dmitryax approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@jcchavezs jcchavezs Awaiting requested review from jcchavezs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rrupapara @dmitryax @carlosalberto @bogdandrutu @jcchavezs @rmfitzpatrick @hprajapati-splunk